ROX-34138: Make HAProxy configurable#240
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change centralizes command configuration assembly, moves the config-file flag to the root command, adds configurable HAProxy bind ports, propagates Roxie settings through subshell execution, and records the active HAProxy port for Central connection output. ChangesConfiguration and HAProxy command flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Command
participant runCommandOrSubshell
participant tryStartHAProxy
participant startHAProxy
participant HAProxy
Command->>runCommandOrSubshell: RoxieConfig
runCommandOrSubshell->>tryStartHAProxy: CentralDeploymentInfo
tryStartHAProxy->>startHAProxy: Validated startup request
startHAProxy->>HAProxy: Start with rendered config and bind port
HAProxy-->>runCommandOrSubshell: Active HAProxyPort
runCommandOrSubshell-->>Command: Forwarded Central connection
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
51d181c to
301552d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/subshell.go (1)
150-153: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider capturing HAProxy stderr for diagnostics.
cmd.Stderr = nildiscards all HAProxy error output. If HAProxy fails to bind or encounters config errors, there's no diagnostic information. Sincecmd.Start()only verifies the process launched (not that it runs successfully), a silent startup failure would leave the user with a banner advertising a URL that doesn't work.♻️ Proposed refactor: capture stderr for error reporting
cmd := exec.Command("haproxy", "-f", configPath) cmd.Stdin = nil cmd.Stdout = nil -cmd.Stderr = nil +var stderr bytes.Buffer +cmd.Stderr = &stderr if err := cmd.Start(); err != nil { os.Remove(configPath) return nil, "", fmt.Errorf("failed to start haproxy: %w", err) } +// Give HAProxy a moment to fail on config/bind errors. +time.Sleep(200 * time.Millisecond) +if cmd.ProcessState != nil && cmd.ProcessState.Exited() { + os.Remove(configPath) + return nil, "", fmt.Errorf("haproxy exited immediately: %s", stderr.String()) +} centralDeploymentInfo.HAProxyPort = roxieConfig.HAProxy.BindPort🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/subshell.go` around lines 150 - 153, Update the HAProxy command setup around exec.Command to capture stderr instead of discarding it, and use the captured diagnostic output when cmd.Start or subsequent startup handling reports failure. Preserve the existing stdin/stdout behavior while ensuring bind and configuration errors are surfaced to the user rather than leaving a misleading success banner.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/subshell.go`:
- Around line 150-153: Update the HAProxy command setup around exec.Command to
capture stderr instead of discarding it, and use the captured diagnostic output
when cmd.Start or subsequent startup handling reports failure. Preserve the
existing stdin/stdout behavior while ensuring bind and configuration errors are
surfaced to the user rather than leaving a misleading success banner.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 0abbd1c6-ac9a-4d12-bbff-21702a5b8f7c
📒 Files selected for processing (9)
cmd/deploy.gocmd/deploy_test.gocmd/flags.gocmd/main.gocmd/shell.gocmd/subshell.gointernal/deployer/config.gointernal/haproxy/config.gointernal/haproxy/config_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/shell.go
- internal/deployer/config.go
- cmd/deploy.go
- cmd/main.go
e.g., enables things like roxie shell --set roxie.haProxy.bindPort=1234
cmd/flags.go, cmd/main.go: Enable flag inheritance for --config, so that it is available on subcommands. cmd/deploy_test.go: Extract subcommand from root command so that they related a parent/child and therefore parsing of `--config` works as expected.
301552d to
2eff914
Compare
|
@coderabbitai, fresh review please. |
|
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 55 minutes. |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/main.go (1)
98-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass a pointer to
mergo.Mergefor consistency.
mergo.Mergeexpects the source argument to ideally be a pointer to avoid reflection overhead and potential type-matching issues. This also aligns with the usage incmd/config.goand line 122 below.♻️ Proposed fix
- if err := mergo.Merge(config, configFromFile, mergo.WithOverride, mergo.WithoutDereference); err != nil { + if err := mergo.Merge(config, &configFromFile, mergo.WithOverride, mergo.WithoutDereference); err != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/main.go` at line 98, Update the mergo.Merge call in the configuration-loading flow to pass configFromFile as a pointer, matching the pointer-based usage in cmd/config.go and the nearby merge call while preserving the existing override and WithoutDereference options.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/main.go`:
- Around line 86-93: Update the filename handling in the withApplyFn callback so
"-" reads from os.Stdin using io.ReadAll, while other filenames continue through
os.ReadFile. Preserve the existing wrapped error handling and avoid replacing
the stdin path with the platform-specific "/dev/stdin" path.
---
Nitpick comments:
In `@cmd/main.go`:
- Line 98: Update the mergo.Merge call in the configuration-loading flow to pass
configFromFile as a pointer, matching the pointer-based usage in cmd/config.go
and the nearby merge call while preserving the existing override and
WithoutDereference options.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 003c5b4e-2150-4f63-810f-537481ece039
📒 Files selected for processing (13)
cmd/config.gocmd/deploy.gocmd/deploy_test.gocmd/flags.gocmd/main.gocmd/shell.gocmd/subshell.gocmd/teardown.gointernal/deployer/config.gointernal/deployer/constants.gointernal/haproxy/config.gointernal/haproxy/config_test.gointernal/types/central_deployment_info.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/haproxy/config.go
- cmd/shell.go
- internal/haproxy/config_test.go
- cmd/deploy_test.go
- cmd/flags.go
- internal/deployer/config.go
- cmd/teardown.go
- cmd/subshell.go
|
@coderabbitai, #240 (review) is addressed. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh api repos/stackrox/roxie/pulls/240/reviews --jq '.[] | select(.id==4692369489) | {id, body, state}'
gh api repos/stackrox/roxie/pulls/240/reviews/4692369489/comments --jq '.[] | {path, line, body}'Length of output: 7714 🏁 Script executed: #!/bin/bash
sed -n '75,130p' cmd/main.goLength of output: 2495 Both flagged items are fixed: the
|
vladbologa
left a comment
There was a problem hiding this comment.
Looks good, didn't know about the HAProxy support, in fact! 👍
Summary
HAProxyConfigtoRoxieConfigwith a configurable bind port (default 8080) and a disable toggle, replacing the previously hardcoded port and booleanHAProxyStartedflag.--configand--setto persistent root-level flags. This allows all subcommands (includingroxie shell) to use them — e.g.roxie shell --set roxie.haProxy.bindPort=9090— while deduplicating the config assembly logic into a sharedassembleConfigForCommandhelper.internal/haproxypackage.Manual tests
roxie deploy centralspawns HAProxy onlocalhost:8080by default.roxie shell --set roxie.haProxy.bindPort=9090can be used for re-attaching with HAProxy listening on localhost:9090.--set roxie.haProxy.disabled=true.